Repository navigation
Fix Network toolbar and footer overflow on narrow screens - #10057
NemesisDevX wants to merge 3 commits into
Conversation
The Network screen toolbar and the shared footer status line overflowed and overlapped at narrow widths. Network toolbar: below 600 px the search field moves to its own row, and below 900 px the filter field moves to its own row. Layout at 900 px and above is unchanged. Footer (StatusLine, shared by all screens): footer text is truncated instead of overflowing. Below 900 px the video tutorial link and, when DevTools is embedded, the embedded actions move into a More actions menu. Each menu item runs the same action as its inline counterpart. Below 600 px the disconnected status shows an icon with a tooltip instead of text. Fixes flutter#10004 Assisted-by: Devin AI
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves responsiveness on narrow screens for the DevTools status line and the Network screen toolbar, preventing layout overflows and overlaps. It wraps status line items in flexible widgets, moves secondary actions into an overflow menu on compact widths, and wraps the Network screen's search and filter fields onto their own rows when space is limited. Comprehensive responsive layout tests and test utilities are also added. The review feedback suggests using Flexible instead of Expanded for the page status in the status line to maintain proper spacing, and utilizing GlobalKeys for the search and filter fields in the Network screen to preserve widget state and focus during responsive reparenting.
| pageStatus, | ||
| // The page status needs to be flexible so that the status line can | ||
| // shrink to fit narrow screens without overflowing. | ||
| Expanded(child: pageStatus), |
There was a problem hiding this comment.
[CONCERN] Using Expanded forces the pageStatus widget to occupy all remaining horizontal space, which overrides the MainAxisAlignment.spaceBetween alignment of the parent Row. This can cause visual layout inconsistencies and push other footer elements to the extreme edges even when there is plenty of screen width.\n\nUsing Flexible (which defaults to FlexFit.loose) allows the pageStatus to only take up the space it actually needs when it is short, preserving the intended spaceBetween spacing, while still constraining it and allowing it to shrink (preventing overflow) on narrow screens.
| Expanded(child: pageStatus), | |
| Flexible(child: pageStatus), |
There was a problem hiding this comment.
Done — switched to Flexible. With Expanded the page status absorbed all leftover space inside its own box, which changed the even spaceBetween spacing the footer had before this change. Flexible restores the previous wide-screen spacing while still shrinking (and ellipsizing) when space is constrained. Added tests asserting the status keeps its natural width on wide screens and that the connection status stays right-aligned.
Give the Network search and filter fields stable keys so their widget state (text, selection, and focus) survives being reparented between toolbar rows when the window crosses the 600 and 900 pixel breakpoints. Previously each reparent recreated the filter field's text controller and focus, losing the selection and focus, and the remounted search field's autofocus stole focus. Wrap the footer's page status in Flexible instead of Expanded so it keeps its natural width and the status line's spaceBetween alignment keeps spacing even on wide screens, matching the layout before the responsive change. It still shrinks below its natural width, and ellipsizes, when space is constrained. Assisted-by: Devin AI
|
|
||
| // Cross each boundary in both directions in the same tree, and finish | ||
| // on a wide desktop baseline. | ||
| for (final width in [599.0, 600.0, 899.0, 900.0, 899.0, 1400.0]) { |
There was a problem hiding this comment.
I think the tests are a bit redundant: 599 and 600 are both .xs and 899 vs 900 are both .s. You could use 600 and 601, and 900 and 901 to test boundaries, but I don't think its too important.
There was a problem hiding this comment.
Done in 48a8996 — responsiveLayoutWidths is now [328, 600, 601, 900, 1400], keeping the 600/601 boundary pair and dropping 599, 899, 901, and 411. The resize test sequence was updated to the same canonical widths while still crossing each boundary in both directions.
Assisted-by: Devin AI
| debugger = DebuggerController(); | ||
| }); | ||
|
|
||
| for (final embedded in [false, true]) { |
There was a problem hiding this comment.
_NetworkProfilerControlsState in network_screen.dart does not inspect embedMode or IdeTheme.isEmbedded. Running all 9 widths in standalone and all 9 widths in embedded mode (18 tests total) tests identical toolbar layouts.
Let's change this to be one group for standalone, with the 9 widths inside. And then add one test case outside the group for the 'embedded' case, choosing just one width, as a smoke test.
There was a problem hiding this comment.
Done in 48a8996 — the embedded/standalone loop is now a Network toolbar (standalone) group over responsiveLayoutWidths, plus a single embedded smoke test at 328 (the toolbar does not read the embed mode). 18 tests reduced to 6.
Assisted-by: Devin AI
| /// These include the realistic narrow widths from the issue (328 and 411), the | ||
| /// `MediaSize` boundaries used by the Network toolbar and the status line (600 | ||
| /// and 900), and a desktop baseline. | ||
| const responsiveLayoutWidths = <double>[ |
There was a problem hiding this comment.
Let's switch these to 328, 600, 601, 900, and 1400, to make it more concise.
There was a problem hiding this comment.
Done in 48a8996 — responsiveLayoutWidths is now exactly [328, 600, 601, 900, 1400].
Assisted-by: Devin AI
| } | ||
|
|
||
| group('footer layout', () { | ||
| for (final embedded in [false, true]) { |
There was a problem hiding this comment.
These three for-loops make a lot of test cases just for the footer geometry. Disconnected status only changes appearance at width <= MediaSize.xs (icon tooltip vs text). Running disconnected across all 9 widths in both standalone and embedded mode is redundant.
Let's test disconnected at 2 representative widths (e.g. 328 for icon and 1400 for text) rather than 18 times.
There was a problem hiding this comment.
Done in 48a8996 — the three-mode x 9-width matrix is collapsed: standalone+connected geometry runs once over the 5 canonical widths, embedded is a single 328 smoke test, and disconnected now runs at exactly two representative widths (328 asserting the icon-with-tooltip, 1400 asserting the text), covering the MediaSize.xs icon-vs-text switch.
Assisted-by: Devin AI
| } | ||
| } | ||
|
|
||
| for (final width in [328.0, 600.0, 901.0, 1400.0]) { |
There was a problem hiding this comment.
These tests at these 4 widths are identical to what the Network footer tests already verify. A single test verifying that a screen without a pageStatus behaves properly in StatusLine is sufficient.
There was a problem hiding this comment.
Done in 48a8996 — the per-width Logging-footer tests are removed and replaced with a single 'footer for a screen without a page status lays out cleanly' test at 328, using a fake screen whose buildStatus returns null.
Assisted-by: Devin AI
| ], | ||
| child: wrapWithControllers( | ||
| DevToolsScaffold( | ||
| screens: [NetworkScreen(), LoggingScreen()], |
There was a problem hiding this comment.
Since this pumpDevToolsScaffold function boots up a complete DevToolsScaffold, NetworkScreen, and LoggingScreen and because NetworkController starts active HTTP/socket polling timers, every single test in status_line_test.dart has to remember to call await stopNetworkPolling(tester, network);.
I think since StatusLine is a lightweight StatelessWidget, it can be tested in isolation using wrapWithControllers without instantiating NetworkScreen or needing stopNetworkPolling.
There was a problem hiding this comment.
Done in 48a8996 — StatusLine tests now pump an isolated Scaffold(bottomNavigationBar: StatusLine(...)) via wrapWithControllers. A new _FakeStatusScreen provides the page status without looking up a screen controller, so NetworkController is never initialized, no polling timers start, and every stopNetworkPolling call is gone. AnalyticsController and ReleaseNotesController are provided above the app (as in pumpDevToolsScaffold) so dialogs opened from the footer can still read them.
Assisted-by: Devin AI
| /// | ||
| /// Launching is intercepted at the `url_launcher` method channel, so the test | ||
| /// checks the URL without opening a browser. | ||
| class UrlLaunchRecorder { |
There was a problem hiding this comment.
This is great. Please add a TODO comment noting that we should use this to test other external links, and maybe move to packages/devtools_test/lib/src/helpers/url_launcher.dart if we use it more generally.
There was a problem hiding this comment.
Done in 48a8996 — TODO added on UrlLaunchRecorder noting reuse for other external links and a possible move to packages/devtools_test/lib/src/helpers/url_launcher.dart.
Assisted-by: Devin AI
| /// | ||
| /// The analytics implementation used in tests logs each selection, so the | ||
| /// events are captured from the log rather than from a network request. | ||
| class AnalyticsRecorder { |
There was a problem hiding this comment.
This is great; I think other tests have fake analytics controllers; can you add a TODO that this could be moved to packages/devtools_test/lib/src/helpers/analytics.dart and used more generally.
There was a problem hiding this comment.
Done in 48a8996 — TODO added on AnalyticsRecorder noting it could move to packages/devtools_test/lib/src/helpers/analytics.dart and be shared with other tests that have fake analytics controllers.
Assisted-by: Devin AI
|
|
||
| /// Asserts that the text in [finder] is laid out at its full intrinsic width, | ||
| /// so that it is not ellipsized or wrapped. | ||
| void expectTextNotTruncated(WidgetTester tester, Finder finder) { |
There was a problem hiding this comment.
This can also be more generally used. Please add a TODO comment noting it can be used to test ellipsis behavior in tabs, bread crumbs, status lines, and table cells, and can be moved to packages/devtools_test/lib/src/helpers/.
There was a problem hiding this comment.
Done in 48a8996 — TODO added on expectTextNotTruncated noting it can test ellipsis behavior in tabs, bread crumbs, status lines, and table cells, and could move to packages/devtools_test/lib/src/helpers/ if used more generally.
Assisted-by: Devin AI
| ); | ||
| }); | ||
|
|
||
| group('Network toolbar behavior', () { |
There was a problem hiding this comment.
I think this group can be removed; it's redundant with other tests:
- The search, filter, and recording logic is already thoroughly covered in
network_screen_test.dartandnetwork_profiler_test.dart. - Hit-testability of the controls is already verified by
_expectToolbarLayoutviaexpectInteractable. - Repeating text entry and record toggling across three widths tests Flutter's TextField and button mechanics rather than responsive layout logic.
There was a problem hiding this comment.
Done in 48a8996 — the 'Network toolbar behavior' group is removed; search/filter/recording coverage stays in network_screen_test.dart and network_profiler_test.dart, and hit-testability remains covered by expectInteractable inside _expectToolbarLayout.
Assisted-by: Devin AI
Reduce the responsive test matrix to the representative widths suggested in review (328, 600, 601, 900, 1400), keep full standalone width coverage for the Network toolbar and footer, and collapse embedded coverage to a single smoke test since the toolbar does not read the embed mode. Consolidate StatusLine geometry and disconnected-state tests, retaining icon-vs-text assertions at the xs boundary and the no-page-status case. Replace the pumpDevToolsScaffold setup for the isolated StatusLine tests with wrapWithControllers over a fake screen whose page status needs no screen controller, so no NetworkController is initialized and no polling timers need cleanup. Drop the toolbar behavior group already covered by the Network screen's own tests, keep a single PopupMenuButton selection per menu item, and add TODOs for the reusable URL launcher, analytics, and text-ellipsis helpers. Assisted-by: Devin AI
Fixes #10004
The Network screen toolbar and the shared footer status line overflowed and overlapped at narrow widths (328 to 599 px). The toolbar's search and filter fields were squeezed into one row beside fixed controls. The footer's page status, device text and embedded actions did not fit.
What changes
StatusLine, shared by all screens): footer text is truncated with an ellipsis instead of overflowing. Below 900 px the video tutorial link and, when DevTools is embedded, the Settings, Feedback, About and extension-settings actions move into a "More actions" popup. Each item calls the same callback as its inline control, and the tutorial item records the same analytics event. Below 600 px the disconnected status shows an icon with a tooltip instead of text.DevToolsScaffold.defaultActionsnow returnsList<ScaffoldAction>(wasList<Widget>), so the footer can reuse the existing actions. The other caller inapp.dartis unchanged.Reviewer notes:
Before and after
Before and after captures at 328 px and 411 px, and separate 1400 px desktop comparisons, were taken with headless Chromium against the upstream base (
9ac04e2) and this branch, using the same networking test app. They are attached below as public GitHub attachments.Attachments, all public GitHub user attachments. The GIF and images were captured by scripts that drove headless Chromium against real pages. This is agent-operated capture, not manual testing.
Corrected 9.8-second GIF. Three scenes: 328 px standalone, 411 px embedded footer, and the More actions menu open at 411 px. Each frame shows the upstream base on the left and this branch on the right.
328 px standalone. Base on the left, this branch on the right.
411 px embedded footer. Base on the left, this branch on the right.
411 px embedded, More actions menu open. The popup overlaps part of the footer while it is open.
1400 px desktop, standalone. Base on top, this branch below. The top toolbar band and the request details pane are pixel-identical between builds. The footer request count differs with traffic.
1400 px desktop, embedded. Base on top, this branch below.
Tests
Run on the repository-pinned candidate SDK (Flutter
3.50.0-0.2.pre, revisionfab991537bf). Results below are split by commit because the test selection changed when the review follow-up added tests.Initial commit
3d3804d:flutter testonstatus_line_test.dartandnetwork_responsive_test.dartflutter teston framework, network, logging, extension, memory diff and property editor suitesdart format --set-exit-if-changed(changed files)dart analyze(changed files) anddart analyze libdt presubmit(without--fix)flutter_lintsresolution warnings in fixture format checks.Review follow-up commit
416fe20(addresses the two review comments:Flexiblepage status, stable keys for the search/filter fields):flutter testonstatus_line_test.dartandnetwork_responsive_test.dartflutter testontest/framework,test/screens/networkandtest/screens/loggingdart analyze(changed files)dt presubmit(without--fix)dart analyze --fatal-infosanddart format --set-exit-if-changedacross the repo. Warnings: nonfatal analytics network exceptions, andflutter_lintsresolution warnings in fixture format checks.flutter test test/(fulldevtools_appsuite at416fe20)+634 ~9 -1before hanging indeep_links_screen_test; attempt B reached+1229 ~12 -1before hanging intreemap_test.dart(a golden test). The observed failure was the CPU-profile benchmark threshold (GenerateFromCpuSamplesBenchmark, expected under 120000, actual ~155000-247000). An earlier full-suite run on the upstream base9ac04e2also failed (+1649 ~12 -1), while the same benchmark passed standalone on the base in seconds. An environment- or load-related cause is plausible but not proven. Neither attempt completed, so no final exit code exists; the earlier disclosed intermittent "used after being disposed" error was not observed. CI is the reference for the full suite.Not run:
integration_test/tests and macOS or Windows desktop builds. Golden coverage was not successfully completed: a golden test (treemap_test.dart) was reached during the full-suite attempt but stalled before finishing.Known limitations (pre-existing, not changed here)
PopupMenuButtonbehaves the same on this SDK. The widget tests cover arrow keys under the Flutter test binding, not in a browser.AI assistance disclosure
Implementation and tests were developed with AI assistance (Devin, an AI software agent), which also ran the scripted browser checks. GPT-6, an AI model, reviewed the change and the evidence. The contributor directed and supervised this work, has reviewed the submitted changes, understands them, and takes responsibility for them. An earlier series of agent-authored development commits is not part of this PR. This PR contains two commits:
3d3804d, the original fix, and416fe20, a follow-up addressing the two automated review comments.Checklist
Pre-launch checklist (boxes left unchecked are for the contributor to confirm personally):
General checklist:
///).Issues checklist:
contributions-welcomeorgood-first-issuelabel. (Not applicable: DevTools Network tab header and footer toolbar controls overlap on narrow screens #10004 has neither label.)contributions-welcomeorgood-first-issuelabel. I understand this means my PR might take longer to be reviewed. (DevTools Network tab header and footer toolbar controls overlap on narrow screens #10004's labels aretype-bug,devtools-network-screen,P2.)Tests checklist:
AI-tooling checklist:
Feature-change checklist:
packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md.If you need help, consider asking for help on Discord.